Lightmode for Query Performance Toast #5004
Conversation
WalkthroughThe changes involve modifications to two components: Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/packages/app/src/components/QueryPerformanceToast.tsx (3)
Line range hint 46-48: Consider consolidating styles in theme configuration
The inline styles and repeated theme references could be simplified by moving common values to theme constants.
Consider refactoring to use sx prop consistently:
- style={{ marginLeft: "auto", color: theme.text.secondary }}
+ sx={{ marginLeft: "auto", color: theme.text.secondary }}Also, consider creating theme constants for commonly used style combinations:
const toastButtonStyles = {
marginLeft: "auto",
color: theme.text.secondary,
};Also applies to: 67-67
Line range hint 43-106: Enhance accessibility for toast notifications
The toast component could benefit from improved accessibility features to better support screen readers and keyboard navigation.
Consider adding these accessibility enhancements:
<Toast
+ role="alert"
+ aria-live="polite"
duration={SHOWN_FOR}
layout={{ bottom: '100px', vertical: "bottom", horizontal: "center", backgroundColor: theme.custom.toastBackgroundColor}}
// ... rest of the props
>Also consider:
- Adding keyboard shortcuts for dismissing the toast (e.g., Escape key)
- Ensuring proper focus management when the toast appears
- Adding aria-label to the Bolt icon for screen readers
Line range hint 35-38: Consider adding error boundary protection
While the event listener cleanup is properly handled, the component could benefit from better error handling.
Consider wrapping the component with an error boundary to gracefully handle rendering failures:
class QueryPerformanceToastErrorBoundary extends React.Component {
componentDidCatch(error: Error) {
console.error('Failed to render QueryPerformanceToast:', error);
// Optional: report to error tracking service
}
render() {
return this.props.children;
}
}
// Usage:
export default function QueryPerformanceToastWithErrorBoundary() {
return (
<QueryPerformanceToastErrorBoundary>
<QueryPerformanceToast />
</QueryPerformanceToastErrorBoundary>
);
}app/packages/components/src/components/ThemeProvider/index.tsx (1)
96-97: Use consistent HSL format for color values.
While the changes look good, the toastBackgroundColor uses hex format (#FFFFFF) which is inconsistent with the HSL format used throughout the theme. Consider using HSL for consistency.
custom: {
lightning: "hsl(25, 100%, 51%)",
- toastBackgroundColor: "#FFFFFF",
+ toastBackgroundColor: "hsl(0, 0%, 100%)",
},📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/packages/app/src/components/QueryPerformanceToast.tsx (1 hunks)
- app/packages/components/src/components/ThemeProvider/index.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/app/src/components/QueryPerformanceToast.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/components/src/components/ThemeProvider/index.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/app/src/components/QueryPerformanceToast.tsx (1)
24-24: Good improvement to initial state management!
Setting the initial state to false prevents unwanted flash of content and aligns with toast notification best practices.
app/packages/components/src/components/ThemeProvider/index.tsx (1)
96-97: LGTM! Changes align with light mode support.
The color changes appropriately support the light mode for Query Performance Toast:
- Changed lightning color to a more maintainable HSL format
- Updated toastBackgroundColor to white for light mode visibility
CamronStaley
force-pushed
the
bug/qp-lightmode
branch
from
3506dae
to
06e17aa
Compare
What changes are proposed in this pull request?
Adding light mode support
How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes